Skip to content

Conversation

giladchase
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@giladchase giladchase marked this pull request as ready for review September 16, 2025 13:18
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from f840c46 to bcf3c7d Compare September 18, 2025 07:11
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from d2487b7 to 235db8b Compare September 18, 2025 07:11
Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@TomerStarkware reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from 235db8b to eeaa0bf Compare September 18, 2025 08:17
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from bcf3c7d to 04b763d Compare September 18, 2025 08:17
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 04b763d to c03411e Compare September 28, 2025 06:43
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch 2 times, most recently from 45abe52 to afd7694 Compare September 28, 2025 06:44
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch 2 times, most recently from 8885cbb to a4243a8 Compare September 28, 2025 08:26
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch 2 times, most recently from 9ab073e to b38e4cc Compare September 28, 2025 12:30
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from a4243a8 to 6769953 Compare September 28, 2025 12:30
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from b38e4cc to 20ea89a Compare September 28, 2025 13:00
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 6769953 to 19ce8b6 Compare September 28, 2025 13:00
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from 20ea89a to bc5dfbf Compare September 29, 2025 06:44
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 19ce8b6 to 4bf4932 Compare September 29, 2025 06:44
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @TomerStarkware)


corelib/src/byte_array.cairo line 703 at r4 (raw file):

    fn at(self: @ByteSpan, index: usize) -> Option<u8> {
        let actual_index = index.checked_add(upcast(self.first_char_start_offset))?;
        let (word_index, index_in_word) = DivRem::div_rem(actual_index, BYTES_IN_BYTES31_NONZERO);

use bounded_int::div_rem and get index_in_word in [0,30] range.
you can use bounded_int::constrain later to decide if it is accessing the lower or upper word.

Code quote:

let (word_index, index_in_word) = DivRem::div_rem(actual_index, BYTES_IN_BYTES31_NONZERO);

corelib/src/byte_array.cairo line 713 at r4 (raw file):

                if word_index == self.data.len() && index_in_word < upcast(self.remainder_len) {
                    // index_in_word is from MSB, we need index from LSB.
                    let index_in_remainder = upcast(self.remainder_len) - 1 - index_in_word;

bounded_int::sub as well.

Code quote:

 let index_in_remainder = upcast(self.remainder_len) - 1 - index_in_word;

@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from bc5dfbf to 50fdc8e Compare September 29, 2025 07:26
Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@TomerStarkware reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed 1 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)


corelib/src/byte_array.cairo line 917 at r6 (raw file):

    }

    pub fn bytespan_get_usize(self: @ByteSpan, index: usize) -> Option<u8> {

doc and improve name.

@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from e37a182 to a29cd34 Compare October 16, 2025 04:42
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from 1d9e119 to 0760802 Compare October 16, 2025 04:42
Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/byte_array.cairo line 917 at r6 (raw file):

Previously, orizi wrote…

doc and improve name.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase changed the base branch from gilad/09-14-feat_byte_array_add_slice_ to graphite-base/8427 October 19, 2025 11:27
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from a29cd34 to 2280b52 Compare October 19, 2025 12:36
@giladchase giladchase changed the base branch from graphite-base/8427 to gilad/09-14-feat_byte_array_add_slice_ October 19, 2025 12:37
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 2280b52 to 02d1817 Compare October 20, 2025 06:23
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from fdf41e3 to b557e75 Compare October 20, 2025 06:23
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed 2 of 3 files at r9, all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)


corelib/src/byte_array.cairo line 1014 at r9 (raw file):

    // For byte_at: div_rem of (usize + BoundedInt<0,30>) by 31
    const USIZE_PLUS_30_DIV_31: felt252 = 0x8421085; // (usize::MAX + 30) / 31 = 138547333

should work.

Suggestion:

((Bounded::<usize>::MAX + 30) / 31).into();

@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 02d1817 to 826e33e Compare October 20, 2025 08:16
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch 2 times, most recently from 5148fd0 to 2dd37d7 Compare October 20, 2025 09:28
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 826e33e to 1b13525 Compare October 20, 2025 09:28
Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)


corelib/src/byte_array.cairo line 1014 at r9 (raw file):

Previously, orizi wrote…

should work.

Messes up semantic:

thread '<unnamed>' panicked at crates/cairo-lang-semantic/src/items/constant.rs:839:19:
Variant extract failed: `Extern(ExternFunctionId(f830))` is not of variant `GenericFunctionId::Impl`

When debugging it i switched it the +30 to -30 and it passed semantic (but failed in the logic of course), meaning probably semantic is having problems deciding what type to evaluate this to (maybe it tries to evaluate the type after the addition before taking the Div into account?).
Maybe this is a bug in semantic.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @giladchase and @TomerStarkware)


corelib/src/byte_array.cairo line 1014 at r9 (raw file):

Previously, giladchase wrote…

Messes up semantic:

thread '<unnamed>' panicked at crates/cairo-lang-semantic/src/items/constant.rs:839:19:
Variant extract failed: `Extern(ExternFunctionId(f830))` is not of variant `GenericFunctionId::Impl`

When debugging it i switched it the +30 to -30 and it passed semantic (but failed in the logic of course), meaning probably semantic is having problems deciding what type to evaluate this to (maybe it tries to evaluate the type after the addition before taking the Div into account?).
Maybe this is a bug in semantic.

lets go with `(Bounded::::MAX / 31 + 1).into(); instead.

@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 1b13525 to ec6fe60 Compare October 20, 2025 12:56
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from 2dd37d7 to e75d288 Compare October 20, 2025 12:56
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from ec6fe60 to 2a75931 Compare October 20, 2025 13:24
Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)


corelib/src/byte_array.cairo line 1014 at r9 (raw file):

Previously, orizi wrote…

lets go with `(Bounded::::MAX / 31 + 1).into(); instead.

Nice, done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @TomerStarkware)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed 1 of 1 files at r11.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants